Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve troubleshooting guide for issues switching from Docker Engine #34764

Closed
wants to merge 6 commits into from

Conversation

nitishfy
Copy link
Member

@nitishfy nitishfy commented Jul 1, 2022

Fix: #34756

Note
The approach that I followed was to add a small paragraph at the beginning of the page (https://kubernetes.io/docs/tasks/debug/debug-cluster/) Cluster not starting which will guide the users to navigate to the following page (https://kubernetes.io/docs/tasks/administer-cluster/migrating-from-dockershim/) to understand more about the error: Error: failed to parse kubelet flag: unknown flag: --network-plugin

Also, Before upgrading to Kubernetes 1.24, it is essential to go through Urgent Upgrade Notes . This could also be seen from @sftim comment on the issue that this PR is fixing.

Please feel free to suggest changes if necessary.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. language/en Issues or PRs related to English language labels Jul 1, 2022
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Jul 1, 2022
@netlify
Copy link

netlify bot commented Jul 1, 2022

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 8187003
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/63e3cc97870d220007aad079
😎 Deploy Preview https://deploy-preview-34764--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@divya-mohan0209 divya-mohan0209 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few grammatical nits + incorporated suggestions from @Sea-n

content/en/docs/tasks/debug/debug-cluster/_index.md Outdated Show resolved Hide resolved
content/en/docs/tasks/debug/debug-cluster/_index.md Outdated Show resolved Hide resolved
content/en/docs/tasks/debug/debug-cluster/_index.md Outdated Show resolved Hide resolved
@nitishfy
Copy link
Member Author

nitishfy commented Jul 4, 2022

Hi @divya-mohan0209 @Sea-n , Thank you for suggesting the changes. The following changes have been added now.

@nitishfy nitishfy requested review from divya-mohan0209 and Sea-n July 5, 2022 15:35
@@ -14,7 +14,23 @@ problem you are experiencing. See
the [application troubleshooting guide](/docs/tasks/debug/debug-application/) for tips on application debugging.
You may also visit the [troubleshooting overview document](/docs/tasks/debug/) for more information.

Before upgrading to Kubernetes 1.24 , please go through [Urgent Upgrade Notes](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.24.md#urgent-upgrade-notes).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: You can remove the word, 'please'.
Before you upgrade to Kubernetes 1.24, review the Urgent Upgrade Notes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should incorporate this feedback @nitishkumar06

Also, we ought to remove this line on the dev-1.25 branch, once that branch has merged in this update.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the intent here, but the advice is misleading enough that we should not merge this as-is.

@nitishkumar06 do you understand how to fix those concerns?

kubelet --help | grep network-plugin
```

*Note: If you get the following error after upgrading to Kubernetes 1.24, go through the [Migrating from dockershim](/docs/tasks/administer-cluster/migrating-from-dockershim/) guide to get a better understanding of how to resolve this error.*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*Note: If you get the following error after upgrading to Kubernetes 1.24, go through the [Migrating from dockershim](/docs/tasks/administer-cluster/migrating-from-dockershim/) guide to get a better understanding of how to resolve this error.*
If you get the following error after upgrading to Kubernetes 1.24, and you were previously using Docker Engine as your container runtime, go through the [Migrating from dockershim](/docs/tasks/administer-cluster/migrating-from-dockershim/) guide.


*Note: If you get the following error after upgrading to Kubernetes 1.24, go through the [Migrating from dockershim](/docs/tasks/administer-cluster/migrating-from-dockershim/) guide to get a better understanding of how to resolve this error.*

```shell
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a shell script. Omit shell here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue is still not addressed.

Comment on lines 22 to 26
To start your cluster, run the following command:

```shell
kubelet --help | grep network-plugin
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a command that starts your cluster

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sftim , I'll push the new changes now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sftim The new changes have been pushed. I've deleted the following command which is wrong and believe it can go without mentioning on "How to start a cluster" since it's error resolving page and not "guide page"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect
You do not start your cluster with kubelet --help | grep network-plugin

/hold
Holding until this is corrected

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 11, 2022
@nitishfy
Copy link
Member Author

WDYT of this change now? @sftim

@sftim
Copy link
Contributor

sftim commented Jul 15, 2022

/retitle Improve troubleshooting guide for issues switching from Docker Engine

@k8s-ci-robot k8s-ci-robot changed the title Cluster not starting error after Kubernetes 1.24 resolved Improve troubleshooting guide for issues switching from Docker Engine Jul 15, 2022
@sftim
Copy link
Contributor

sftim commented Jul 15, 2022

To me, #34764 (comment) is only barely a nit. It really would be better to remove “please”; that word isn't right for a troubleshooting guide.

However, this change is much more right than wrong.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2022
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 0f4c2bfcef3c6289f1170bc6dca3e506eb6ea149

@nitishfy
Copy link
Member Author

@sftim Ahan sorry! I forgot to do that. I'll do it now.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2022
@k8s-ci-robot k8s-ci-robot requested a review from sftim July 15, 2022 16:50
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@nitishfy
Copy link
Member Author

@sftim The requested changes are applied now. I think this PR is ready to be merged. WDYT?

@Sea-n
Copy link
Member

Sea-n commented Sep 20, 2022

Hey @nitishkumar06,

It's been a while since the last update, should we close this PR?

If you want to change the target branch to dev-1.24, it's not necessary to open a new PR, just see this tutorial.

@nitishfy
Copy link
Member Author

Hey @Sea-n @sftim , Do you have further suggestions to go for? I believe we can consider this PR as approved if no one has much suggestions regarding the same.

Comment on lines 17 to 18
Before upgrading to Kubernetes 1.24, please go through [Urgent Upgrade Notes](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.24.md#urgent-upgrade-notes).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current main branch is for 1.25
I'm also ok with this line not being in the top of this page as there is no other content/context around troubleshooting upgrades.
Maybe a warning like this (sans version) can go in the Upgrade a Cluster. A similar sentence is already in the Upgrading kubeadm clusters page

Suggested change
Before upgrading to Kubernetes 1.24, please go through [Urgent Upgrade Notes](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.24.md#urgent-upgrade-notes).
Before upgrading to Kubernetes 1.24, please go through [Urgent Upgrade Notes](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.24.md#urgent-upgrade-notes).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reylejano Thank you for the suggestions! I'd be committing the changes soon.

@reylejano
Copy link
Member

reylejano commented Sep 30, 2022

Hey @Sea-n @sftim , Do you have further suggestions to go for? I believe we can consider this PR as approved if no one has much suggestions regarding the same.

@nitishkumar06
This PR is not considered approved as there are comments not addressed and an incorrect instruction on how to start a cluster

@reylejano
Copy link
Member

If you want to change the target branch to dev-1.24, it's not necessary to open a new PR, just see this tutorial.

The target branch for the 1.24 docs is the release-1.24 branch

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 12, 2022
@nitishfy
Copy link
Member Author

@reylejano Please checkout the file changes in the latest commit.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from sftim by writing /assign @sftim in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 1, 2023
@nitishfy
Copy link
Member Author

nitishfy commented Jan 1, 2023

@sftim @reylejano @Sea-n Kindly take a look at the changes now.

Copy link
Contributor

@divya-mohan0209 divya-mohan0209 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some suggestions. WDYT?

@@ -27,6 +27,8 @@ the documentation for the version of Kubernetes that you plan to upgrade to.

## Upgrade approaches

please go through [Urgent Upgrade Notes](https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.24.md#urgent-upgrade-notes).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've a couple of suggestions here.

  1. I'd recommend not using the word please here. 2. We're linking to the 1.24 upgrade notes for a particular reason right? We should say why we're asking our audience to go through the guide as a form of context setting.


## {{< note >}}
*Note: Due to change in CNI, the `kubelet` binary might not recognize `--network-plugin` flag even after passing to `kubelet` which leads to the following error:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this could be phrased better. WDYT?

Suggested change
*Note: Due to change in CNI, the `kubelet` binary might not recognize `--network-plugin` flag even after passing to `kubelet` which leads to the following error:
*Note: Starting 1.24, since management of the CNI is no longer in scope for kubelet, the `--network-plugin` command line parameter has been removed from the binary and will throw the following error, if used:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per @reylejano suggested here, I've omitted this line.

content/en/docs/tasks/debug/debug-cluster/_index.md Outdated Show resolved Hide resolved
@kbhawkey
Copy link
Contributor

Hello @nitishkumar06 .
I think it makes sense to close this pull request. You can always create another PR with updated changes. Thanks.
/close

@k8s-ci-robot
Copy link
Contributor

@kbhawkey: Closed this PR.

In response to this:

Hello @nitishkumar06 .
I think it makes sense to close this pull request. You can always create another PR with updated changes. Thanks.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@nitishfy
Copy link
Member Author

How does it make sense to open a new PR? This PR already had the suggested changes pushed. We were looking forward to getting a review from the maintainers before proceeding further rather than closing this one.

@kbhawkey
Copy link
Contributor

Hello @nitishkumar06 . You can always reopen a pull request. Do you want to mark this PR as a draft or are the changes ready to merge?

Copy link
Contributor

@tengqm tengqm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we have removed the heading 2 at line 29.
I'm not sure why we add a heading 3 at line 317.
Line 319-325 is a note, that is fine. But line 319 and 325 are marked heading 2?
Why line 320 starts with a '*'?
Can we wrap the lines inside the note?

@nitishfy
Copy link
Member Author

This workspace kind of looks messed up - Hence created a new PR: #39625

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Signpost readers migrating from dockershim to migration guide (troubleshooting page)
8 participants